[python][scikit-learn] Fixes a bug that prevented using multiple eval_metrics in LGBMClassifier #3222
Conversation
giresg
requested review from
chivee,
guolinke,
henry0312,
jameslamb,
Laurae2,
StrikerRUS and
wxchan
as code owners
| @@ -922,3 +922,14 @@ def test_continue_training_with_model(self): | |||
| self.assertEqual(len(init_gbm.evals_result_['valid_0']['multi_logloss']), 5) | |||
| self.assertLess(gbm.evals_result_['valid_0']['multi_logloss'][-1], | |||
| init_gbm.evals_result_['valid_0']['multi_logloss'][-1]) | |||
|
|
|||
| def test_eval_metrics_lgbmclassifier(self): | |||
There was a problem hiding this comment.
Will it make sense to include the content of this test into already existing huge unit test for metrics as an enhancement for the case of ... invalid multiclass metric is replaced with binary alternative ...
LightGBM/tests/python_package_test/test_sklearn.py
Lines 749 to 753 in 2792923
Or just please move this new test closer to that one because they are both about the same things.
There was a problem hiding this comment.
@StrikerRUS yes definitely makes sense. I've moved the test inside of test_metrics but placed it next to the section
non-default metric with multiple metrics in eval_metric
as it is highly related to that test. Thanks!
| X_classification, y_classification = load_breast_cancer(True) | ||
| params_classification = {'n_estimators': 2, 'verbose': -1, 'objective': 'binary', 'metric': 'binary_logloss'} | ||
| params_fit_classification = {'X': X_classification, 'y': y_classification, | ||
| 'eval_set': (X_classification, y_classification), 'verbose': False} |
There was a problem hiding this comment.
Please fix one linting error:
./tests/python_package_test/test_sklearn.py:547:23: E128 continuation line under-indented for visual indent
There was a problem hiding this comment.
Thank you! Sorry for the inconvenience, but could you please rebase to the latest master? I've just fixed R-package builds: #3225. Those failures prevents from merging.
There was a problem hiding this comment.
Rebased, hopefully I did it correctly as it is my first time rebasing an upstream branch 😄
There was a problem hiding this comment.
Thanks!
But seems that something went wrong: see number of changed files
Maybe you can allow editing from maintainers and then I'll be able to help with cleaning up the rebasing?
https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork
There was a problem hiding this comment.
There was a problem hiding this comment.
No problem! Seems I was able to exclude excess files.
StrikerRUS
force-pushed
the
fix-lgbmclassifier-eval-metric
branch
from
0c53735
to
5bcc9ec
Compare
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
This PR fixes a bug in the LGBMClassifier class. The code as-is breaks when eval_metric is a list of strings. This is meant to work (as referenced in the documentation) and does work on all derived classes of LGBMModel except LGBMClassifier. The issue is in the following code in the master branch:
LightGBM/python-package/lightgbm/sklearn.py
Lines 785 to 798 in 1e2013a
with the fix the following line works as expected (see tests in PR):
gbm = lgb.LGBMClassifier(**params).fit(eval_metric=['fair', 'error'], **params_fit)